-
Notifications
You must be signed in to change notification settings - Fork 341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make bool values lowercase in solr query url - fixes #401 #435
base: master
Are you sure you want to change the base?
Make bool values lowercase in solr query url - fixes #401 #435
Conversation
@@ -300,6 +303,16 @@ def __iter__(self): | |||
result = result._next_page_query and result._next_page_query() | |||
|
|||
|
|||
def get_nested(obj, keys, default=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is only used by a debug logging call, and it could be replaced by a standard dictionary get-with-default call there.
@@ -528,7 +541,7 @@ def _update( | |||
path_handler = handler | |||
if self.use_qt_param: | |||
path_handler = "select" | |||
query_vars.append("qt=%s" % safe_urlencode(handler, True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be unrelated to the proposed change.
# cover both cases: there is no response key or value is None | ||
(decoded.get("response", {}) or {}).get("numFound", 0), | ||
) | ||
if decoded.get("grouped"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would defer support for grouped responses to a larger task which would provide more help for them - just changing a log message doesn't seem to add much value.
@@ -72,6 +73,9 @@ def test_safe_urlencode(self): | |||
"test=Hello \u2603!&test=Helllo world!", | |||
) | |||
|
|||
# Boolean options for Solr should be in lowercase. | |||
self.assertTrue("True" not in safe_urlencode({"group": True})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would pass a bug which encoded it as group=None
or group="false"
.
self.assertTrue("True" not in safe_urlencode({"group": True})) | |
self.assertEqual("group=true", safe_urlencode({"group": True})) |
@@ -191,6 +191,9 @@ def safe_urlencode(params, doseq=0): | |||
which can't fail down to ascii. | |||
""" | |||
if IS_PY3: | |||
for key, val in params.items(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, pysolr has avoided doing type coercion to avoid needing to know what type(s) Solr supports for a given field (e.g. a parameter like group
is documented as accepting true
but will also accept on
or yes
but not 1
or True
) but I think this is relatively safe because Solr doesn't accept that value natively so the only case where this could cause a problem is if someone was passing a Solr string which they for some reason wanted to have processed literally — for example, if I had a Python app which used a Solr StringField for something and expected the literal value True
or False
because that field type isn't case-insensitive.
Minor changes as titled. Also added a small util function to look up nested json objects.